Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Report all lints, even if other errors already occurred. #108813

Closed
wants to merge 1 commit into from
Closed

Report all lints, even if other errors already occurred. #108813

wants to merge 1 commit into from

Conversation

oli-obk
Copy link
Contributor

@oli-obk oli-obk commented Mar 6, 2023

This reduces surprises of the "I fixed all the errors, now I'm getting new ones"-kind

r? @ghost

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Mar 6, 2023
@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 6, 2023

@bors try @rust-timer queue

@rust-timer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 6, 2023
@bors
Copy link
Contributor

bors commented Mar 6, 2023

⌛ Trying commit c913bdc2672f0ae9f8a8b18d71171129234b1d53 with merge d9845989f7fe27114bf2d89955cc4fa6bade7a67...

@bors
Copy link
Contributor

bors commented Mar 6, 2023

☀️ Try build successful - checks-actions
Build commit: d9845989f7fe27114bf2d89955cc4fa6bade7a67 (d9845989f7fe27114bf2d89955cc4fa6bade7a67)

1 similar comment
@bors
Copy link
Contributor

bors commented Mar 6, 2023

☀️ Try build successful - checks-actions
Build commit: d9845989f7fe27114bf2d89955cc4fa6bade7a67 (d9845989f7fe27114bf2d89955cc4fa6bade7a67)

@rust-timer

This comment has been minimized.

Sign in to view
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (d9845989f7fe27114bf2d89955cc4fa6bade7a67): comparison URL.

Overall result: no relevant changes - no action needed

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. While you can manually mark this PR as fit for rollup, we strongly recommend not doing so since this PR may lead to changes in compiler perf.

@bors rollup=never
@rustbot label: -S-waiting-on-perf -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 2.7% [2.7%, 2.7%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 2.3% [2.3%, 2.3%] 1
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

@rustbot rustbot removed the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Mar 6, 2023
@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 6, 2023

Okay... not sure why I ran perf at all 😃 it's not like perf benchmarks emit errors 🤦

@matthiaskrgr
Copy link
Member

Well it kind makes sense to have some warning/error related benchmarks (perhaps even clippy lints 📎 ? )
I have seen code where rustc would print a couple of initial errors, and then "freeze" for a couple of seconds (4-10?) just to not print any further errors which always seemed a bit weird 🙃

@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 7, 2023

r? compiler

Will bless the test later today

@lcnr
Copy link
Contributor

lcnr commented Mar 7, 2023

how likely is it that we report lints which are invalid because of a previous error? Currently slightly overwhelmed with my other reviews, so

r? compiler

@rustbot rustbot assigned compiler-errors and unassigned lcnr Mar 7, 2023
@oli-obk
Copy link
Contributor Author

oli-obk commented Mar 7, 2023

how likely is it that we report lints which are invalid because of a previous error?

We should fix those lints by checking for error types or errored typeck in case that happens. I found no obviously wrong cases in the changed diagnostics

@cjgillot
Copy link
Contributor

cjgillot commented Mar 7, 2023

Type checking errors still early-exit because of the ? on rustc_hir_analysis::check_crate a few lines above.
Removing all the short-circuits in that function open floodgates of a larger magnitude.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Mar 13, 2023

☔ The latest upstream changes (presumably #108471) made this pull request unmergeable. Please resolve the merge conflicts.

riking
riking reviewed Mar 15, 2023
View reviewed changes
tests/ui/consts/const-mut-refs/mut_ref_in_final.stderr
LL | const B: *mut i32 = &mut 4;
| ^^^^^^^^^^^^^^^^^

error: untyped pointers are not allowed in constant
Copy link

@riking riking Mar 15, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This lint is clearly incorrect here: needs an bail for pointer to Err?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not incorrect, but the wording can use some improvements. Basically only raw pointers that are just integers with a raw pointer type are allowed. All other raw pointers are forbidden.

@rustbot
Copy link
Collaborator

rustbot commented Mar 16, 2023

These commits modify the Cargo.lock file. Random changes to Cargo.lock can be introduced when switching branches and rebasing PRs.
This was probably unintentional and should be reverted before this PR is merged.

If this was intentional then you can ignore this comment.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Apr 6, 2023

☔ The latest upstream changes (presumably #108504) made this pull request unmergeable. Please resolve the merge conflicts.

@compiler-errors
Copy link
Member

@oli-obk what's the status on this? do you want me to review it, or is does it need reworking? does it need an FCP? 🤔

@oli-obk
Copy link
Contributor Author

oli-obk commented Apr 19, 2023

I guess I could rebase it and then compiler FCP it. Not sure it needs an FCP, i'll bring it up in the T-compiler triage

@oli-obk oli-obk added the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Apr 19, 2023
@wesleywiser
Copy link
Member

I think either an FCP or MCP would be fine 🙂

jyn514
jyn514 approved these changes Apr 20, 2023
View reviewed changes
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good idea and would second an MCP.

@apiraino apiraino removed the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Apr 25, 2023
@apiraino apiraino added the needs-mcp This change is large enough that it needs a major change proposal before starting work. label May 10, 2023
@apiraino
Copy link
Contributor

switching review flag to note that an MCP would be fine in this case (Zulip mention)

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 10, 2023
@oli-obk oli-obk mentioned this pull request May 11, 2023
Closed
3 tasks
@oli-obk
Report all lints, even if other errors already occurred.
f77c183 
This reduces surprises of the "I fixed all the errors, now I'm getting new ones"-kind
@bors
Copy link
Contributor

bors commented May 16, 2023

☔ The latest upstream changes (presumably #111639) made this pull request unmergeable. Please resolve the merge conflicts.

@JohnCSimon
Copy link
Member

@oli-obk
ping from triage - can you post your status on this PR? There hasn't been an update in a few months, and I see a few merge conflicts Thanks!

@WaffleLapkin
Copy link
Member

@JohnCSimon Oli is currently on leave, I think they'll continue working on this once they are back in January =)

@oli-obk
Copy link
Contributor Author

oli-obk commented Jan 3, 2024

The MCP has not been approved and has concerns that can only be resolved by a larger discussion (RFC?) about whether we want to report all lints and errors upfront, or do it in a phased system like we do now.

@oli-obk oli-obk closed this Jan 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@riking riking riking left review comments

@jyn514 jyn514 jyn514 approved these changes

Assignees

@compiler-errors compiler-errors

Labels
needs-mcp This change is large enough that it needs a major change proposal before starting work. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.